Skip to content

[Data] Change PhysicalOperator.completed() to has_completed()#59234

Merged
bveeramani merged 12 commits intoray-project:masterfrom
simeetnayan81:rename-completed
Dec 30, 2025
Merged

[Data] Change PhysicalOperator.completed() to has_completed()#59234
bveeramani merged 12 commits intoray-project:masterfrom
simeetnayan81:rename-completed

Conversation

@simeetnayan81
Copy link
Contributor

@simeetnayan81 simeetnayan81 commented Dec 7, 2025

Description

This is a follow-up PR after: #58915 for changing the function name from completed() to has_completed() in the class PhycicalOperator. This is in-line with our discussions with @bveeramani here: #58915 (review). This is done to make it clear that the method doesn't modify state and has no side effects.

Related issues

#58884

Additional information

Just updated the completed() and all it's references in other classes and tests.

… it's references

Signed-off-by: Simeet Nayan <simeetnayan.8100@gmail.com>
@simeetnayan81 simeetnayan81 requested review from a team as code owners December 7, 2025 05:11
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to rename the PhysicalOperator.completed() method to has_completed() for clarity, along with updating all its call sites. The changes are extensive and cover many files, which is great. I've reviewed the changes and they are mostly correct. However, I found one instance in a subclass where the method was overridden but not renamed, which could lead to incorrect behavior. Please see my specific comment for details.

@ray-gardener ray-gardener bot added data Ray Data-related issues community-contribution Contributed by the community labels Dec 7, 2025
Signed-off-by: Simeet Nayan <simeetnayan.8100@gmail.com>
Copy link
Member

@bveeramani bveeramani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you!

@gvspraveen
Copy link
Contributor

@simeetnayan81 Thanks for your contributions. Can you please fix the build failures?

@gvspraveen gvspraveen added the @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. label Dec 11, 2025
@simeetnayan81
Copy link
Contributor Author

@gvspraveen @bveeramani Hey, I've fixed the issues causing tests to fail. Kindly review once and merge.

@bveeramani bveeramani changed the title Change PhysicalOperator.completed() to has_completed() and update all… [Data] Change PhysicalOperator.completed() to has_completed() Dec 12, 2025
@bveeramani bveeramani enabled auto-merge (squash) December 12, 2025 09:09
@bveeramani
Copy link
Member

LGTM! Thank you for the contribution!

@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Dec 12, 2025
@bveeramani bveeramani self-assigned this Dec 26, 2025
@github-actions github-actions bot disabled auto-merge December 26, 2025 05:07
@bveeramani bveeramani merged commit f85c525 into ray-project:master Dec 30, 2025
6 checks passed
AYou0207 pushed a commit to AYou0207/ray that referenced this pull request Jan 13, 2026
…y-project#59234)

## Description

This is a follow-up PR after: ray-project#58915 for changing the function name from
```completed()``` to ```has_completed()``` in the class
PhycicalOperator. This is in-line with our discussions with @bveeramani
here:
ray-project#58915 (review).
This is done to make it clear that the method doesn't modify state and
has no side effects.

## Related issues
ray-project#58884

## Additional information
Just updated the completed() and all it's references in other classes
and tests.

---------

Signed-off-by: Simeet Nayan <simeetnayan.8100@gmail.com>
Co-authored-by: Balaji Veeramani <balaji@anyscale.com>
Signed-off-by: jasonwrwang <jasonwrwang@tencent.com>
lee1258561 pushed a commit to pinterest/ray that referenced this pull request Feb 3, 2026
…y-project#59234)

## Description

This is a follow-up PR after: ray-project#58915 for changing the function name from
```completed()``` to ```has_completed()``` in the class
PhycicalOperator. This is in-line with our discussions with @bveeramani
here:
ray-project#58915 (review).
This is done to make it clear that the method doesn't modify state and
has no side effects.


## Related issues
ray-project#58884

## Additional information
Just updated the completed() and all it's references in other classes
and tests.

---------

Signed-off-by: Simeet Nayan <simeetnayan.8100@gmail.com>
Co-authored-by: Balaji Veeramani <balaji@anyscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Contributed by the community data Ray Data-related issues @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. go add ONLY when ready to merge, run all tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants